Conversation
chaiminwoo0223
left a comment
There was a problem hiding this comment.
코드 리뷰 작성했습니다. 확인 부탁드립니다.
| Trip trip = tripService.getValidTrip(member.getId(), tripId); | ||
|
|
||
| DailyGoal dailyGoal = dailyGoalService.getValidDailyGoal(trip, dailyGoalId); | ||
|
|
There was a problem hiding this comment.
updateDailyGoal, deleteDailyGoal, getDailyGoal 메서드에서 동일한 로직이 반복되고 있습니다.
공통된 로직은 private 메서드로 분리하여 재사용하는 것이 좋을 것 같아요.
예를 들어, private DailyGoal getValidDailyGoalFromTripOwnedByMember
There was a problem hiding this comment.
updateDailyGoal() 메서드는 Trip 정보도 필요합니다.
DailyGoalFacade 클래스 내부에
예를 들어, private record TripWithDailyGoal(Trip trip, DailyGoal dailyGoal) {} 의 record 를 만들고, private TripWithDailyGoal getTripAndDailyGoalOfMember() 메서드로 중복을 제거하는 방법은 어떨까요?
There was a problem hiding this comment.
record를 별도로 만들 경우 오히려 구조가 복잡해질 것 같아요.
모든 메서드에서 반복되는 부분(Member, Trip)만 private 메서드로 분리하는 것은 어떨까요?
private Trip getValidTripOwnedByMember(Long memberId, Long tripId) {
Member member = memberService.getMember(memberId);
Trip trip = tripService.getValidTrip(member.getId(), tripId);
return trip
}
There was a problem hiding this comment.
record를 별도로 만들 경우 오히려 구조가 복잡해질 것 같아요.
모든 메서드에서 반복되는 부분(
Member,Trip)만 private 메서드로 분리하는 것은 어떨까요?private Trip getValidTripOwnedByMember(Long memberId, Long tripId) { Member member = memberService.getMember(memberId); Trip trip = tripService.getValidTrip(member.getId(), tripId); return trip }
넵, 좋은 것 같아요.
공통 로직만 깔끔하게 private 메서드로 분리하는 방식이 훨씬 가독성 면에서 좋을 것 같아요.
record까지 포함되면 오히려 코드가 복잡해 보일 수 있겠다는 생각이 듭니다
| import com.ject.studytrip.pomodoro.domain.error.PomodoroErrorCode; | ||
| import com.ject.studytrip.pomodoro.domain.model.Pomodoro; | ||
|
|
||
| public class PomodoroPolicy { |
There was a problem hiding this comment.
Policy 클래스에 @NoArgsConstructor(access = AccessLevel.PRIVATE)를 추가하여, 외부에서 불필요하게 인스턴스를 생성하지 못하도록 막는 것이 좋을 것 같아요.
정적 메서드만 사용하는 유틸리티 성격의 클래스에서는 @NoArgsConstructor(access = AccessLevel.PRIVATE)를 통해, 의도를 명확히 전달하고 잘못된 사용을 방지하는데 도움을 줍니다.
| import org.springframework.validation.annotation.Validated; | ||
| import org.springframework.web.bind.annotation.*; | ||
|
|
||
| @Tag(name = "DailyGoal", description = "오늘의 목표 API") |
There was a problem hiding this comment.
오늘의 목표 API보다 데일리 목표 API로 description 내용을 변경하는 것이 더 좋을 것 같아요.
|
|
||
| public record CreateDailyGoalRequest( | ||
| @Schema(name = "뽀모도로") @Valid @NotNull(message = "뽀모도로 정보는 필수 요청 값입니다.") | ||
| CreatePomodoroRequest pomodoro, |
There was a problem hiding this comment.
DailyGoalController.createDailyGoal()에서도 @Valid 어노테이션을 사용하셨는데,CreateDailyGoalRequest에서 @Valid 어노테이션을 중복 사용하신 이유가 있을까요?
지금 요청 dto 구조가 CreateDailyGoalRequest 안에 CreatePomodoroRequest 객체가 포함되어 있는 구조입니다.
DailyGoalController 에서 CreateDailyGoalRequest DTO에 @Valid 어노테이션을 적용하면 CreateDailyGoalRequest의 필드에 선언된 제약 조건(@NotNull 등)은 검증되지만,
중첩된 CreatePomodoroRequest 객체 내부의 필드 제약 조건까지는 자동으로 검증되지 않는 걸로 알고 있습니다.
중첩된 CreatePomodoroRequest 내부 필드 제약 조건까지 검증하기 위해 추가로 @Vaild 어노테이션을 적용했습니다
There was a problem hiding this comment.
확인했습니다. 말씀해주신 것처럼 중첩된 객체의 필드까지 유효성 검증이 필요한 경우,
내부 필드에도 @Valid를 별도로 적용해야 한다는 점 참고하겠습니다.
| import io.swagger.v3.oas.annotations.media.Schema; | ||
| import java.util.List; | ||
|
|
||
| public record UpdateDailyGoalRequest( |
There was a problem hiding this comment.
PATCH 요청을 통해 원하는 값만 선택적으로 수정할 수 있다면, @JsonInclude(JsonInclude.Include.NON_NULL) 을 추가하는 것이 좋을 것 같아요.
@JsonInclude(JsonInclude.Include.NON_NULL) 을 사용하면 null인 필드는 JSON 직렬화 과정에서 자동으로 제외되므로, 서버에서는 실제로 전달된 값만을 기준으로 업데이트 로직을 수행할 수 있어, 선택적 필드 수정과 보다 깔끔한 요청 구조를 구현할 수 있습니다.
There was a problem hiding this comment.
이 부분에 대해 궁금한 점이 있어서 문의드려요
@JsonInclude는 자바 객체를 JSON으로 변환하는 직렬화 과정에서 동작하며 주로 응답 DTO에서 사용한다고 알아 봤습니다
요청 DTO에서는 클라이언트로부터 전달받은 JSON을 자바 객체로 변환하는 역직렬화를 수행하는데, 이 경우에도 @JsonInclude가 적용되고 동작하는지가 궁금합니다
There was a problem hiding this comment.
제가 잘못 알고 있었네요.
송현님 말씀대로 @JsonInclude 어노테이션은 주로 응답 DTO에서 사용되며, 서버가 JSON으로 응답할 때 null인 필드를 제외하는 역할을 합니다. 반면, 요청 DTO에서는 역직렬화 과정이므로, 클라이언트가 null을 보내더라도 Java 객체에는 그대로 매핑됩니다.
There was a problem hiding this comment.
제가 잘못 알고 있었네요.
송현님 말씀대로
@JsonInclude어노테이션은 주로응답 DTO에서 사용되며, 서버가 JSON으로 응답할 때 null인 필드를 제외하는 역할을 합니다. 반면,요청 DTO에서는 역직렬화 과정이므로, 클라이언트가null을 보내더라도 Java 객체에는 그대로 매핑됩니다.
@JsonInclude 어노테이션에 대해 알려주셔서 저도 직렬화와 역직렬화 과정에서 Jackson 동작 방식에 대해 알게 되었어요
우리 서비스에서 null 값은 클라이언트 응답에 포함되지 않는 게 더 적절한 경우가 있으면 @JsonInclude을 적용해 더 깔끔한 JSON 응답을 만들 수 있을 것 같아요
37d202d to
45823f1
Compare
chaiminwoo0223
left a comment
There was a problem hiding this comment.
고생하셨습니다. 머지 부탁드립니다.
* feat: DailyGoal, Pomodoro, DailyMission 관련 ErrorCode, Policy, Factory 클래스 추가 * feat: 데일리 목표 생성 기능 구현 (Pomodoro 및 DailyMission 함께 생성) * feat: 데일리 목표 수정 기능 구현 (새로운 DailyMission 추가 및 기존 항목 삭제 지원) * feat: 데일리 목표 삭제 시 관련 Pomodoro, DailyMission 함께 삭제 처리 * feat: 특정 데일리 목표 조회 시 관련 Pomodoro, DailyMission 함께 조회 * feat: 미션 목록 조회 시 관련 Stamp를 fetch join으로 함께 조회하는 쿼리 추가 * feat: Stamp 정보와 함께 미션을 조회하고 검증된 미션 목록을 반환하는 로직 구현 * feat: 미션 에러코드, 검증 메서드 추가 * feat: 코스형 여행에서 현재 진행중인 스탬프를 조회하는 쿼리 및 로직 추가 * test: DailyGoal, Pomodoro, DailyMission Fixture 및 Helper 클래스 작성 * test: DailyGoalController 통합 테스트 작성 * test: DailyGoal, Pomodoro, DailyMission 서비스 단위 테스트 작성 * test: Mission 서비스 단위 테스트 코드 추가 (미션과 스탬프를 함께 조회하고 검증된 미션 목록 반환 테스트) * test: Stamp 서비스 단위 테스트 코드 추가 (코스형 여행에서 현재 진행중인 스탬프 조회 테스트)
📌 작업 내용 및 특이사항
✅ 데일리 목표 생성 API
뽀모도로(Pomodoro)와미션(Mission)정보를 같이 전달하고뽀모도로(Pomodoro),데일리 미션(DailyMission)을 함께 생성-> 현재는 뽀모도로 생성 요청 시
focusDurationInMinute(집중 시간)만 입력받지만, 추후 고도화에 따라 필드 확장을 고려completed필드가false인 가장 첫번째 스탬프)하고 해당 스탬프에 속한 미션들인지 검증✅ 데일리 목표 수정 API
PATCH요청으로 원하는 필드만 요청✅ 데일리 목표 삭제 API
deletedAt필드를 요청한 시간으로 수정해Soft Delete수행뽀모도로(Pomodoro),데일리 미션(DailyMission)도 함께 논리 삭제되도록 구성✅ 특정 데일리 목표 조회 API
DailyGoal정보를 조회하는 기능 구현뽀모도로(Pomodoro),데일리 미션(DailyMission)정보들도 함께 반환하도록 구성✅ Mission, Stamp 서비스 로직 추가
Stamp를fetch join해 조회하고, 검증 후 유효한 미션 목록을 반환하는 로직 추가Stamp(아직 완료되지 않은 가장 첫 번째Stamp)를 조회하는 로직 추가:
completed필드가false인 가장 첫번째Stamp를 조회하도록 구성✅ fetch join 쿼리 구성
Lazy loading인 상황에서mission.getStamp()를 매번 하게되면 호출할 때마다 불필요한 쿼리가 발생하는N+1문제가 발생하고 이를 방지하고자fetch join를 사용했습니다.Mission정보를 함께 가공하는데 이 경우에도DailyMission -> Mission관계에서fetch join을 적용해 불필요한 쿼리 발생을 방지했습니다.✅ 테스트
DailyGoal,Pomodoro,DailyMissionFixture및Helper클래스 작성DailyGoalController통합 테스트 작성:
DailyGoal서비스 단위 테스트 작성:
Pomodoro서비스 단위 테스트 작성:
DailyMission서비스 단위 테스트 작성:
Mission서비스 단위 테스트에 미션과 스탬프를fetch join하고 검증된 미션 목록을 조회하는 테스트 작성:
Stamp서비스 단위 테스트에 코스형 여행일 경우 현재 진행중인 스탬프를 조회하는 테스트 작성🌱 관련 이슈
🔍 참고사항(선택)
현재 피그마에는 데일리 목표 수정 화면이 없어, 임의로 UI 흐름을 구성한 상태입니다. 추후 변경사항이 생기면 즉시 반영하겠습니다.
데일리 목표 수정 API에서는
Pomodoro정보는 수정하지 않도록 구성했습니다.피그마 상에서는
Pomodoro시간을 탭하면 모달을 통해 수정하는 방식으로 되어 있어,Pomodoro수정은 별도의 API로 분리해도 될 것 같다고 생각했습니다. 이 부분도 추후 변경사항이 생기면 즉시 반영하겠습니다.📚 기타(선택)